Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scripts Refactor #296

Merged
merged 57 commits into from
Mar 26, 2024
Merged

Scripts Refactor #296

merged 57 commits into from
Mar 26, 2024

Conversation

leeyi45
Copy link
Contributor

@leeyi45 leeyi45 commented Mar 20, 2024

The first rewriting of the module scripts left a lot to be desired. With the benefit of new packages and lots of hindsight, here is a streamlined set of scripts that should resolve the issues where linting errors were not being properly caught by the CI.

Currently there's a really weird bug that continues to escape me with the testing for the json command, so I have that disabled. It's not really too important this instant, but it would be nice to figure out why Jest isn't calling the mocked function.

This refactor also includes some changes to how modules are loaded and how the JSON documentation is stored. I'll have the corresponding PRs in js-slang up.

Copy link
Member

@RichDom2185 RichDom2185 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just did a quick run through – perhaps we can add "newline at EOF" as an error rule in .eslintrc.base.cjs?

devserver/src/components/Playground.tsx Show resolved Hide resolved
scripts/jest.config.js Outdated Show resolved Hide resolved
scripts/jest.config.js Outdated Show resolved Hide resolved
scripts/jest.setup.ts Outdated Show resolved Hide resolved
@leeyi45
Copy link
Contributor Author

leeyi45 commented Mar 22, 2024

With the new flat configurations that ESLint is moving towards, I basically redid the ESLint setup for the repository. It should be far less janky than the way it had been setup previously and should play nicely with the VSCode extension (if you use the defaults provided in the .vscode folder)

Copy link
Member

@RichDom2185 RichDom2185 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semicolons are still not checked (some files have them, some files don't, and some have a mix)

@@ -1,6 +1,7 @@
/**
* This file contains the bundle's representation of GameObjects.
*/
import Phaser from 'phaser';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to find where this import is used but I can't find it (and yet VSCode shows that it is not an unused import)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused by this one to be honest

@martin-henz martin-henz added the critical [Priority] Fixing this is mission-critical label Mar 26, 2024
@martin-henz
Copy link
Member

The current sourceacademy.org frontend needs this PR to run modules.

Copy link
Member

@RichDom2185 RichDom2185 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@RichDom2185 RichDom2185 enabled auto-merge (squash) March 26, 2024 12:18
@RichDom2185 RichDom2185 enabled auto-merge (squash) March 26, 2024 12:45
@RichDom2185 RichDom2185 merged commit 0128673 into master Mar 26, 2024
3 checks passed
@RichDom2185 RichDom2185 deleted the scripts-refactor branch March 26, 2024 12:49
@RichDom2185 RichDom2185 mentioned this pull request Mar 26, 2024
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
critical [Priority] Fixing this is mission-critical
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants